Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

When is this going to be merged? Seems like this is capable of solving some significant challenges.

@blakeoxx I tried this and didn't work as expected. For example, setting cookie in SsrCookieServices using the code does not set it. Do you have an example where it shows this code working?

@pavankjadda try out this test repo: https://github.com/blakeoxx/ngx-cookie-service-18-test. You can build and start the project with npm run build followed by npm run serve:ssr:ssr-cookie-test.

I generated this project with Angular's ng new --ssr command, added the scaffolding to server.ts as outlined in our README, and added a test to the AppComponent which outputs console logs on the server console and client/browser console to confirm the server-side cookie is readable and writable by both platforms. Example output.

Additionally, something I noticed was that NG18's SSR defaults to using prerendering. I had to disable this in the builder architect configuration. It's intended behavior that SSR won't run on prerendered routes and not a bug of this code, but perhaps may be worth calling out in the README.

Let me know if you need anything else or don't see expected results.

This package seems to be highly needed within the Angular community, but I’ve noticed that this PR has been open for some time. I personally have a strong need for this functionality and tried to implement it in Angular 17, but it doesn’t seem to be working.

In my setup, I’m saving the token on the server using ngx-cookie-service, but when I try to retrieve it on the client side, the token is null.

@blakeoxx, do you happen to have any working demos for Angular 17 with a workflow similar to mine?

In advance, thanks for all your hard work on this project—it’s much appreciated!

I’d like to take this opportunity to share what I’m trying to achieve in my app, so other users with similar goals can benefit as well.

In my setup, I have a forwarded host from server.ts, and in my app, my goal is to:

1. Make a request to my backend based on the host received.
2. Save the token and some configuration data from the response to cookies using ngx-cookie-service-ssr.

I expect the browser to load this saved data without making new requests. However, when I try to retrieve the token and configuration, they’re all null.

Here's my server.ts for reference

import 'zone.js/node';

import { APP_BASE_HREF } from '@angular/common';
import { CommonEngine } from '@angular/ssr';
import * as express from 'express';
import { existsSync } from 'node:fs';
import { join } from 'node:path';
import AppServerModule from './src/main.server';
import { REQUEST as SSR_REQUEST } from 'ngx-cookie-service-ssr';

// The Express app is exported so that it can be used by serverless Functions.
export function app(): express.Express {
    const server = express();
    const distFolder = join(process.cwd(), 'dist/tennistool/browser');
    const indexHtml = existsSync(join(distFolder, 'index.original.html'))
        ? join(distFolder, 'index.original.html')
        : join(distFolder, 'index.html');

    const commonEngine = new CommonEngine();

    server.set('view engine', 'html');
    server.set('views', distFolder);

    // Example Express Rest API endpoints
    // server.get('/api/**', (req, res) => { });
    // Serve static files from /browser
    server.get(
        '*.*',
        express.static(distFolder, {
            maxAge: '1y',
        })
    );

    // All regular routes use the Angular engine
    server.get('*', (req, res, next) => {
        const { protocol, originalUrl, baseUrl, headers } = req;

        commonEngine
            .render({
                bootstrap: AppServerModule,
                documentFilePath: indexHtml,
                url: `${protocol}://${headers.host}${originalUrl}`,
                publicPath: distFolder,
                providers: [
                    { provide: APP_BASE_HREF, useValue: baseUrl },
                    { provide: SSR_REQUEST, useValue: req },
                    { provide: 'RESPONSE', useValue: res },
                    
                    // harcoded host for testing
                    {
                        provide: 'X_FORWARDED_HOST',
                        useValue: 'sub.mydomain.com',
                    },
                ],
            })
            .then((html) => res.send(html))
            .catch((err) => next(err));
    });

    return server;
}

function run(): void {
    const port = process.env['PORT'] || 4000;

    // Start up the Node server
    const server = app();
    server.listen(port, () => {
        console.log(`Node Express server listening on http://localhost:${port}`);
    });
}

// Webpack will replace 'require' with '__webpack_require__'
// '__non_webpack_require__' is a proxy to Node 'require'
// The below code is to ensure that the server is run only when not requiring the bundle.
declare const __non_webpack_require__: NodeRequire;
const mainModule = __non_webpack_require__.main;
const moduleFilename = (mainModule && mainModule.filename) || '';
if (moduleFilename === __filename || moduleFilename.includes('iisnode')) {
    run();
}

export default AppServerModule;

Thanks for your kind words, @betojsx ! Just glad to help :)

do you happen to have any working demos for Angular 17 with a workflow similar to mine?

This PR is intended for use with NG18 projects. I'm assuming there are some compatibility issues between the Angular internals. Have you tried using the v17 artifact referenced in this comment? If that doesn't resolve your issue, let me know and I'll be happy to create a demo repo for v17 as well.

Have you tried using the v17 artifact referenced in #266 (comment)

Yep! I forgot to mention that I already have the v17 artifact in my project.

"ngx-cookie-service-ssr": "github:blakeoxx/ngx-cookie-service-ssr-artifact#v17"

I really appreciate your effort to help. At this point, I’ve tried everything I could but still haven’t figured out how to make it work, so I’ll wait for some examples

Looking over your server.ts again, I originally missed that you don't have the response token in your providers. This token is necessary for the package to write cookies on the HTTP response sent to the client. Try updating your server.ts with the following:

import { REQUEST as SSR_REQUEST, RESPONSE as SSR_RESPONSE } from 'ngx-cookie-service-ssr';
// ...
commonEngine
            .render({
                // ...
                providers: [
                    { provide: APP_BASE_HREF, useValue: baseUrl },
                    { provide: SSR_REQUEST, useValue: req },
                    { provide: SSR_RESPONSE, useValue: res },
                    { provide: 'RESPONSE', useValue: res },
                    
                    // harcoded host for testing
                    {
                        provide: 'X_FORWARDED_HOST',
                        useValue: 'sub.mydomain.com',
                    },
                ],
            })

OMG @blakeoxx, you solved it!

That was exactly the issue! I can’t thank you enough. 🙏

I spent countless hours debugging this, even put the project on hold for a while out of frustration. I’m so glad I decided to share it here, and even more thankful for your help. You’ve saved me a ton of time!

@pavankjadda any updates on your review of this PR? Anything needed to keep this moving?

Haven't got time. Will try to do this week.

Hey, anything new on that?

i wonder if we add code like below and access this.rawCookie instead of this.document.cookie

  get rawCookie(): string | undefined {
    if (this.documentIsAccessible) {
      return this.document.cookie;
    } else {
      const responseCookies = this.response?.get('Set-Cookie');
      let respCookies = '';
      if (responseCookies) {
        respCookies = Array.isArray(responseCookies) ? responseCookies.join(';') : responseCookies;
        return this.request?.headers.cookie + ';' + respCookies;
      } else {
        return this.request?.headers.cookie;
      }
    }
  }

We probably only need change Set method.

i wonder if we add code like below and access this.rawCookie instead of this.document.cookie

  get rawCookie(): string | undefined {
    if (this.documentIsAccessible) {
      return this.document.cookie;
    } else {
      const responseCookies = this.response?.get('Set-Cookie');
      let respCookies = '';
      if (responseCookies) {
        respCookies = Array.isArray(responseCookies) ? responseCookies.join(';') : responseCookies;
        return this.request?.headers.cookie + ';' + respCookies;
      } else {
        return this.request?.headers.cookie;
      }
    }
  }

We probably only need change Set method.

@maxisam I think one potential issue I see with this code is the case of the cookie being set in the request as well as response. In this code, the cookie would be duplicated, causing the first instance from the request to be the value always returned by this.get. This is the main reason why I elected to use a key-value Map in this.getCombinedCookies. I also think names and values of cookies from the request/response headers may be URI encoded since they may be returned by the express interface as one concatenated string.

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

That is a good point. Do you think reverse this will solve this issue? If we have duplicated cookie, I think the response one will overwrite the request one in the next request.

respCookies + ';' + this.request?.headers.cookie

Don't get me wrong, I think using map is fine. My first thought was using Map to handling cookies as well, but if we add "string to map" solution, it seems like we have duplicated way to handle cookie.

I feel like it is easier to maintain if we can consolidate this into one solution. Either way is fine I think.

I do think switching the order would fix the issue. However, I'd caution that it might be surprising behavior to future maintainers that cookies appear multiple times in the string and only the first occurrence is used due to the specific coding of this.get.

In the end, I also don't feel that strongly about either implementation. If you feel it would ease maintainability, I'll happily update. Do you think the maintainability boost your solution offers is worth the surprising behavior?

well, this will be @pavankjadda 's decide. Either way is fine with me, but i do think having one solution is probably better than having both.